Skip to content

Conversation

@OS-pedrolourenco
Copy link
Contributor

Issue number: internal


What is the current behavior?

The ion-range pin is only shown we the range is being dragged for all themes.

What is the new behavior?

  • This PR updates the range pin so that it is alway visible when it is enabled for the ionic theme.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@OS-pedrolourenco OS-pedrolourenco requested a review from a team as a code owner October 31, 2024 19:01
@vercel
Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 5, 2024 2:46pm

@github-actions github-actions bot added the package: core @ionic/core package label Oct 31, 2024
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of my requested changes are dependent on the --height variable being set to 40px as requested on #29979. This will make the total range height equal to 68px, matching Figma.

Copy link
Member

@brandyscarney brandyscarney Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 66 should be:

@include padding(calc(globals.$ion-space-100 + globals.$ion-scale-600), null, null, null);

This will make it so the padding top is the height of the pin text + the space between the knob and pin text

# Conflicts:
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Chrome-linux.png
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Firefox-linux.png
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Safari-linux.png
Copy link
Contributor

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion to eliminate changes in the component

{pin && (
<div class="range-pin" role="presentation" part="pin">
{pinFormatter(value)}
{pinFormatter(value) + (theme === 'ionic' ? '%' : '')}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about changing the pinFormatter prop to return number | string. That way, we don't have to add this theme specific logic, and the OS widget could provide its own formatter that would append the %

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great! I will do that

Base automatically changed from ROU-11323 to next November 5, 2024 13:03
@OS-pedrolourenco OS-pedrolourenco changed the base branch from next to main November 5, 2024 13:15
@OS-pedrolourenco OS-pedrolourenco changed the base branch from main to next November 5, 2024 13:15
# Conflicts:
#	core/src/components/range/range.ionic.scss
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Chrome-linux.png
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Firefox-linux.png
#	core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Safari-linux.png
@tanner-reits
Copy link
Contributor

LGTM!

@OS-pedrolourenco OS-pedrolourenco merged commit c1128dd into next Nov 5, 2024
46 checks passed
@OS-pedrolourenco OS-pedrolourenco deleted the ROU-11324 branch November 5, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants